Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[receiver/awscloudwatch] Fix validation in some scenarios #14953

Merged

Conversation

schmikei
Copy link
Contributor

@schmikei schmikei commented Oct 14, 2022

Description: Since the default config obtains an autodiscover config, the real config loading runs into issues more often than expected. We should remove the constriction so exclusive named config users can use the receiver.

Link to tracking Issue: Resolves #14952

Testing: Existing tests and this is now a valid config:

receivers:
  awscloudwatch:
    region: us-east-2
    logs:
      poll_interval: 10s
      groups:
        named:
          /aws/eks/dev-eks-0/cluster:
            names: [kube-apiserver-audit-ea9c831555adca1815ae04b884b30566]

Documentation: Updated documentation to reflect this change.

@schmikei schmikei marked this pull request as ready for review October 14, 2022 15:57
@schmikei schmikei requested a review from a team October 14, 2022 15:57
@schmikei schmikei requested a review from djaglowski as a code owner October 14, 2022 15:57
@djaglowski
Copy link
Member

Since the default config obtains an autodiscover config, the real config loading runs into issues more often than expected. We should remove the constriction so exclusive named config users can use the receiver.

Am I understanding correctly that autodiscover is never nil, because it is the default? Therefore, is was impossible to use named groups because validation of the config checks that only one mode is used?

@schmikei
Copy link
Contributor Author

schmikei commented Oct 18, 2022

Since the default config obtains an autodiscover config, the real config loading runs into issues more often than expected. We should remove the constriction so exclusive named config users can use the receiver.

Am I understanding correctly that autodiscover is never nil, because it is the default? Therefore, is was impossible to use named groups because validation of the config checks that only one mode is used?

Yes that is my understanding!

I tried to think of other ways to solve this but this felt like the simplest. If there is a preferred way, I'm open to it!

@djaglowski
Copy link
Member

Ok, as proposed the config ends up in an ambiguous state where both autodiscover and named are set.

The correct way to handle this is to implement a custom unmarshal function for the config. Basically, we can check if autodiscover was actually set, and if not, nil out the corresponding struct.

It should look pretty much like this (adapted from fileexporter):

// Unmarshal a confmap.Conf into the config struct.
func (cfg *Config) Unmarshal(componentParser *confmap.Conf) error {
	if componentParser == nil {
		return errors.New("empty config for file exporter")
	}
	// first load the config normally
	err := componentParser.Unmarshal(cfg, confmap.WithErrorUnused())
	if err != nil {
		return err
	}
	// check if user actually specified autodiscover, or if just autoinitialized
	if !componentParser.IsSet("autodiscover") {
		cfg.Autodiscover = nil
	}
	return nil
}

@schmikei
Copy link
Contributor Author

@djaglowski that seemed to be a great solution, added a couple more tests to validate that these changes are safe and resolve the issue

@djaglowski
Copy link
Member

/easycla

@djaglowski djaglowski merged commit e2f8e76 into open-telemetry:main Oct 18, 2022
@schmikei schmikei deleted the fix-validate-of-awscloudwatchreceiver branch October 18, 2022 14:45
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
…etry#14953)

* Fix validation of some awscloudwatch configs that used named groups
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/awscloudwatch] Named config validation failing due to default config
2 participants